Welcome @zuzzas!

It looks like this is your first PR to kubernetes/cloud-provider-vsphere 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-vsphere has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @zuzzas. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test
/assign @andrewsykim @dvonthenen

Thanks! Going to fix tests today.

Some tests have failed, but these are not related to my changes.

We need to carefully think about if those new fields in the config belong in pkg/common level. I need to think about this some more, but I think might initial gut reaction is that this might be the first time we need to create an outer CPI specific config since this might only apply to the CPI.

What do you think @andrewsykim ?

My initial question is if we should accept network names or their subnets 🤔 need to noodle on that one too

My initial question is if we should accept network names or their subnets 🤔 need to noodle on that one too

If we do "VM Network" names (this should be done per VC and not in [global] also), there would be a strict requirement that each NIC must have only a single ipv4 and/or ipv6 bound to it.

there would be a strict requirement that each NIC must have only a single ipv4 and ipv6 bound to it.

So if NICs can be bound to more than 1 address it sounds like it should be based on subnet? @zuzzas thoughts?

Also, if there is only a single ipv4 or ipv6 address on the VM, we shouldn't force/require setting those new config options. It should just work.

We should also move these new config options out of pkg/common because that package is shared with CSI and these new fields have no meaning to CSI. The new config options should be placed in pkg/cloudprovider/vsphere

Fixes #266

I'll refactor the config and contribute to the discussion about network names vs subnet CIDR tomorrow.

I've started to muse about pros and cons of L2 vs L3 matching (network names vs IPs), but the point was moot, since a user might find both approaches desirable, so I've just coded both variants.

I've also created a separate CPI-specific config, but dependency injection of it left a lot to be desired.

The tests were not updated, I'll do them once you, guys, approve of these changes.

I've started to muse about pros and cons of L2 vs L3 matching (network names vs IPs), but the point was moot, since a user might find both approaches desirable, so I've just coded both variants.

My thinking here is that because Kubernetes node addresses are strictly L3, the config we expose to users should also strictly be L3. I'm interested in the use-cases for L2 though -- is it possible to specify a network name that gives me more than 2 addresses for a node? If so what should be the expected behavior?

In regards to the go tests failing... I have a PR to fix one and @codenrhoden is taking a look at the other one. Tracking both in this PR #273

/retest

This should at least pass the CI now we have merged the two PRs to fix the unit and integration tests. Still need to resolve the overall discussion with the PR.

/lgtm

Can you squash commits and use a more descriptive message please?

@andrewsykim
It is done.

/lgtm

Leaving final approval to @dvonthenen

Thanks @zuzzas !

Seriously! Awesome contribution @zuzzas!

/approve
/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dvonthenen

The full list of commands accepted by this bot can be found here.

The pull request process is described here